Add Google Cloud KMS signing capability #442

jku · 2022-10-26T09:36:03Z

Add support for Google Cloud KMS signing. The use case for this is TUF repositories with online keys. Removing the private key material from the repository software is a good thing for security: a compromise of the repository software will then only allow signing during the compromise but won't allow stealing the private keys.

The included Google implementation depends on implicit authentication: the runtime environment should contain authentication credentials that google.cloud.kms library is able to find.

Example of a test run: https://github.com/jku/securesystemslib/actions/runs/3384917757/jobs/5622619919
This uses google-github-actions/auth to authenticate to Google Cloud (this is the implicit authentication part): the github project token is configured to have Signer/Verifier permissions in Google KMS.

The usability should improve if #447 or something like it gets implemented:

the private key URI for this would be something like
gcpkms://projects/python-tuf-kms/locations/global/keyRings/securesystemslib-tests/cryptoKeys/ecdsa-sha2-nistp256/cryptoKeyVersions/1
a function like Key.get_payload_hash_algorithm() would be helpful at that point (needed by signer but it doesn't have to be public)

Please verify and check that the pull request fulfils the following requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

securesystemslib/kms_signer.py

jku · 2022-10-26T11:47:11Z

The discussion that maybe makes sense at this point is private key abstraction in general: I have a dispatch/factory method in KMSSigner but maybe that should be in Signer itself? See full details in #445 (comment)

jku · 2022-10-26T13:03:06Z

securesystemslib/kms_signer.py

+        if key["keytype"] == "ed25519" and key["scheme"] == "ed25519":
+            # specification default is sha-512
+            return "sha512"


I'm not sure where I got this impression from...

Hmm, the spec defines sha512 and the identity function as the prehash options. Using identity (i.e. no hashing) in this case would indeed be silly (we want to send something small over the network)...

However, spec uses "ed25519" to mean the identity prehashed scheme, and "ed25519ph" as the SHA512 prehashed scheme: It's entrirely possible that our current implementation does the former and then we would have to add a new scheme to the ed25519 keytype to signify the prehashed scheme.

Considering that GCP KMS does not even support ed25519, I think I'll just remove this if-clause

jku · 2022-11-02T12:57:54Z

So after a bit of consideration, I think this makes sense:

generic private key improvements are in RFE: private key reference scheme #447: I will remove the whole KMSSigner abstraction from this PR with the assumption that those issues will be solved in 447
testing is still an issue... I will try to find out if there is a low-effort way to get a testing project on GCP

jku · 2022-11-03T09:31:22Z

Oops. I thought I had figured out a way to allow running KMS tests on PRs safely but hit a very practical issue: authenticating to GCP requires the OIDC token from GitHub which understandably isn't given to PRs that come from forks.

So I don't think we can run the KMS test on PRs safely. We can run it on pushes or cron but that's not as nice :(

jku · 2022-11-03T10:50:06Z

OK, tests now only run on push, not on PRs.

Here is the test run on my repo https://github.com/jku/securesystemslib/actions/runs/3384917757/jobs/5622619919 (the GCP setup currently accepts requests from both "jku/securesystemslib" and "secure-systems-lab/securesystemslib")

jku · 2022-11-04T08:13:39Z

Ok, final change: added issue filing on failure (otherwise no-one notices when this fails since it does not run on PRs from forks).

Setting non-draft now.

lukpueh

This is super cool and I have nothing to add, except some very minor nits. Feel free to address or ignore as you see fit. Also, let me know if you want to defer merging until after #456.

.github/workflows/test-kms.yml

securesystemslib/signer.py

jku · 2022-11-21T13:38:47Z

let me know if you want to defer merging until after #456.

I was thinking no need ... but since I'm now going to suggest splitting the code to internal submodules maybe that is indeed a good idea -- let's see how "easy" 456 looks to merge once I get a non-draft version ready

jku · 2022-11-30T13:46:52Z

rebased on both master and privkeyuri (so it's not really 24 commits, don't worry)
Implemented the private key uri "gcpkms": works like charm (with a small tweak to the dispatching)
the KMS test "works on my github project": https://github.com/jku/securesystemslib/actions/runs/3583834641/jobs/6029741653

this is now waiting for #456 merge

This does not change API but makes it easier to implement Signers/Keys in multiple files.

A bare bones Signer for Google Cloud KMS: Private keys live in KMS, signing happens in KMS (although payload hashing happens in Signer). Key creation or import is not supported at this point. A test is added with a few caveats: * dependencies are not added to requirements.txt: this would more than triple the size of requirements-pinned.txt... There is a separate requirements file: this is not ideal but best I could come up with. * Test only works on GitHub (because of the GCP authentication), and only on branches within the upstream repo: not on PRs from forks * Test is run only once: it's a smoke test, not an exhaustive matrix test.

Since the tests run on push, it's not easy to notice a failure. File an issue if it happens.

The GCPSigner that was added is not tested in the standard test run, only when check_kms_signers is explicitly ran. This is because the test will fail everywhere except the projects Github CI. Lower required coverage as a workaround.

jku · 2022-11-30T19:35:32Z

Changes since the first review:

action pinning fixed per review comment
uses priv key uri now (constructor changed accordingly)
signer and key dispatch also tweaked slightly to make it easier to implement signers/keys in other files

Can I ask you to have another look @lukpueh?

.github/workflows/test-kms.yml

lukpueh

Found one more non-blocking nit. Re-approving...

securesystemslib/signer/_gcp_signer.py

jku commented Oct 26, 2022

View reviewed changes

securesystemslib/kms_signer.py Outdated Show resolved Hide resolved

jku commented Oct 26, 2022

View reviewed changes

securesystemslib/kms_signer.py Outdated Show resolved Hide resolved

jku force-pushed the kms-signer branch 2 times, most recently from 2f3fc8f to f9b8380 Compare October 26, 2022 10:28

jku commented Oct 26, 2022

View reviewed changes

This was referenced Oct 26, 2022

should support KMS signing #445

Closed

RFE: private key reference scheme #447

Closed

jku force-pushed the kms-signer branch 4 times, most recently from ba9ffbf to b1b260f Compare November 3, 2022 09:22

jku force-pushed the kms-signer branch from b1b260f to 02e2825 Compare November 3, 2022 10:35

jku force-pushed the kms-signer branch from 02e2825 to 9e51512 Compare November 4, 2022 08:05

jku marked this pull request as ready for review November 4, 2022 08:18

jku changed the title ~~Add KMS signing capability~~ Add Googel Cloud KMS signing capability Nov 4, 2022

jku changed the title ~~Add Googel Cloud KMS signing capability~~ Add Google Cloud KMS signing capability Nov 4, 2022

This was referenced Nov 11, 2022

Feature: Add support for KeyVault for AWS (AWSKMS) repository-service-tuf/repository-service-tuf#24

Open

Use KMS keys once the feature is in securesystemslib jku/tuf-demo#35

Closed

lukpueh approved these changes Nov 21, 2022

View reviewed changes

jku force-pushed the kms-signer branch 2 times, most recently from 4fd174c to ae52e1d Compare November 30, 2022 13:43

jku added 2 commits November 30, 2022 21:21

signer API: Move dispatch table population to init

16734bb

This does not change API but makes it easier to implement Signers/Keys in multiple files.

jku added 2 commits November 30, 2022 21:21

workflows: Create issue if KMS test fails

12e8e8f

Since the tests run on push, it's not easy to notice a failure. File an issue if it happens.

tox: lower required coverage

4a3d8fb

The GCPSigner that was added is not tested in the standard test run, only when check_kms_signers is explicitly ran. This is because the test will fail everywhere except the projects Github CI. Lower required coverage as a workaround.

jku force-pushed the kms-signer branch from ae52e1d to 4a3d8fb Compare November 30, 2022 19:25

jku commented Nov 30, 2022

View reviewed changes

.github/workflows/test-kms.yml Show resolved Hide resolved

workflows: Use hashes for pinning actions

75ba1e5

jku force-pushed the kms-signer branch from e49118b to 75ba1e5 Compare December 1, 2022 08:09

jku mentioned this pull request Dec 2, 2022

New release #475

Closed

lukpueh mentioned this pull request Dec 7, 2022

Add HSMSigner #472

Merged

lukpueh approved these changes Dec 7, 2022

View reviewed changes

securesystemslib/signer/_gcp_signer.py Outdated Show resolved Hide resolved

signer: Fix GCPSigner module docstring

724b905

lukpueh merged commit 51c1e5a into secure-systems-lab:master Dec 12, 2022

lukpueh mentioned this pull request Dec 12, 2022

Add support for DSSE #370

Closed

adityasaky mentioned this pull request Dec 12, 2022

build(deps): bump google-github-actions/auth from 0.8.3 to 1.0.0 #478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Google Cloud KMS signing capability #442

Add Google Cloud KMS signing capability #442

jku commented Oct 26, 2022 •

edited

Loading

jku commented Oct 26, 2022 •

edited

Loading

jku Oct 26, 2022

jku Oct 27, 2022 •

edited

Loading

jku commented Nov 2, 2022

jku commented Nov 3, 2022 •

edited

Loading

jku commented Nov 3, 2022 •

edited

Loading

jku commented Nov 4, 2022

lukpueh left a comment

jku commented Nov 21, 2022

jku commented Nov 30, 2022

jku commented Nov 30, 2022

lukpueh left a comment

Add Google Cloud KMS signing capability #442

Add Google Cloud KMS signing capability #442

Conversation

jku commented Oct 26, 2022 • edited Loading

Please verify and check that the pull request fulfils the following requirements:

jku commented Oct 26, 2022 • edited Loading

jku Oct 26, 2022

Choose a reason for hiding this comment

jku Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

jku commented Nov 2, 2022

jku commented Nov 3, 2022 • edited Loading

jku commented Nov 3, 2022 • edited Loading

jku commented Nov 4, 2022

lukpueh left a comment

Choose a reason for hiding this comment

jku commented Nov 21, 2022

jku commented Nov 30, 2022

jku commented Nov 30, 2022

lukpueh left a comment

Choose a reason for hiding this comment

jku commented Oct 26, 2022 •

edited

Loading

jku commented Oct 26, 2022 •

edited

Loading

jku Oct 27, 2022 •

edited

Loading

jku commented Nov 3, 2022 •

edited

Loading

jku commented Nov 3, 2022 •

edited

Loading